revert: Downgrade NVRx / Upgrade nemo-run#2668
Conversation
📝 WalkthroughWalkthroughA pytest marker decorator Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/functional_tests/training/test_nvrx_straggler.py`:
- Line 241: The test file uses the decorator `@pytest.mark.pleasefixme` but never
imports pytest, causing an F821; add an import statement for pytest (e.g.,
import pytest) near the other test imports so the symbol pytest used by the
decorator is defined and the test module can be imported; update any import
grouping in the file to keep imports organized around the existing test helpers
and fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55988e11-47b5-4f55-b3b2-aeb578c9b3e6
📒 Files selected for processing (1)
tests/functional_tests/training/test_nvrx_straggler.py
| return file_handler | ||
|
|
||
|
|
||
| @pytest.mark.pleasefixme |
There was a problem hiding this comment.
Add missing pytest import for the new decorator
Line 241 introduces @pytest.mark.pleasefixme, but pytest is not imported, which triggers F821 and breaks CI/module import.
✅ Proposed fix
import argparse
import logging
import os
import tempfile
import time
+import pytest
import torch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.pleasefixme | |
| import argparse | |
| import logging | |
| import os | |
| import tempfile | |
| import time | |
| import pytest | |
| import torch |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 241-241: undefined name 'pytest'
(F821)
🪛 GitHub Actions: CICD NeMo
[error] 241-241: ruff: F821 Undefined name 'pytest'.
[warning] 241-241: ruff: Selection E266 has no effect because preview is not enabled.
🪛 Ruff (0.15.2)
[error] 241-241: Undefined name pytest
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional_tests/training/test_nvrx_straggler.py` at line 241, The test
file uses the decorator `@pytest.mark.pleasefixme` but never imports pytest,
causing an F821; add an import statement for pytest (e.g., import pytest) near
the other test imports so the symbol pytest used by the decorator is defined and
the test module can be imported; update any import grouping in the file to keep
imports organized around the existing test helpers and fixtures.
| @@ -238,6 +238,7 @@ def setup_test_logging(log_file: str, rank: int): | |||
| return file_handler | |||
|
|
|||
|
|
|||
| @pytest.mark.pleasefixme | |||
There was a problem hiding this comment.
Let me try reverting that
chtruong814
force-pushed
the
chtruong/skip-nvrx
branch
from
60892f2 to
ea6a7d7
Compare
chtruong814
changed the title
|
Merging now to unblock main branch |
This reverts commit d51e99b. Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: Charlie Truong <chtruong@nvidia.com>
2 participants
What does this PR do ?
revert: Downgrade NVRx / Upgrade nemo-run
The nvrx test has been failing since that got meged. Reverting for now until we can fix it better.
#2623
Changelog
GitHub Actions CI
See the CI sectionin the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.
Before your PR is "Ready for review"
Pre checks:
If you haven't finished some of the above items you can still open "Draft" PR.
Additional Information
Summary by CodeRabbit